Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add optional InstanceID field to log output #46

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

ConorC117
Copy link
Collaborator

Add an optional instanceID field to log output for distinguishing multiple running host-metering instances from each other during development.

Example implementation of adding an optional, custom instanceID to log output for distinguishing multiple running host-metering instances from each other during development.

The instanceID value is an empty string by default and only added to the log output if the string is populated in the config file.

Any feedback on my approach or suggestions for improvements is appreciated!

@ConorC117 ConorC117 self-assigned this Apr 8, 2024
@ConorC117 ConorC117 requested review from major, F-X64 and miyunari April 8, 2024 12:54
Copy link
Collaborator

@F-X64 F-X64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes are looking good.
I've tried you changes locally and the new feature works as expected.
The only thing I'd like to add is test coverage for the prefix within the log output.

@@ -51,7 +51,7 @@ func TestInitLogger(t *testing.T) {
func TestInitLoggerFile(t *testing.T) {
dir := t.TempDir()
path := dir + "/test.log"
InitLogger(path, DebugLevel.String())
InitLogger(path, DebugLevel.String(), "test_instance")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently we only test whether the logger is correctly initialized.
Consider adding a test for the prefix as well, e.g.

	if !strings.HasPrefix(string(data), "[test_instance]") {
		t.Errorf("Expected the output to start with '[test_instance]', but got: %s", string(data))
	}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback and for testing the code. Will make the change!

Add an optional instanceID field to log output for distinguishing multiple
running host-metering instances from each other during development.

Signed-off-by: ccowman <[email protected]>
Copy link
Collaborator

@F-X64 F-X64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test. Changes look good now.

@ConorC117 ConorC117 merged commit 27751a7 into RedHatInsights:main Apr 15, 2024
7 checks passed
@ConorC117 ConorC117 deleted the instanceID branch May 3, 2024 10:53
github-actions bot pushed a commit that referenced this pull request May 27, 2024
# [1.3.0](v1.2.0...v1.3.0) (2024-05-27)

### Features

* add optional InstanceID field to log output ([#46](#46)) ([27751a7](27751a7))
* enable host-metering.service on rpm installation ([d231460](d231460))
Copy link

🎉 This PR is included in version 1.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants